add previous_response_id continuation and responses contract typing#310
add previous_response_id continuation and responses contract typing#310ndycode wants to merge 6 commits intofeat/openai-parity-pr1from
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughadded response continuation feature enabling requests to resume from prior response ids. when enabled via config, the plugin derives a session key, retrieves the last stored response id from session affinity store, injects it into request body, and captures new response ids from sse streams via callbacks. extended session store to track response ids and updated request/response handling across fetch and sse layers. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Plugin as Plugin Handler
participant SessionStore as Session Affinity<br/>Store
participant API as Codex API
participant Parser as SSE Parser
Client->>Plugin: Request (no previous_response_id)
Plugin->>SessionStore: getLastResponseId(sessionKey)
SessionStore-->>Plugin: lastResponseId (if stored & valid)
Plugin->>Plugin: Clone body, inject previous_response_id
Plugin->>API: Fetch with continuation
API-->>Plugin: Response stream
Plugin->>Parser: parseSseStream(stream, onResponseId)
Parser->>Parser: Read SSE events
Parser->>Parser: Extract id from response.done event
Parser->>Plugin: Invoke onResponseId(responseId)
Plugin->>SessionStore: rememberLastResponseId(sessionKey, id)
SessionStore-->>Plugin: Stored
Parser-->>Plugin: Return final response
Plugin-->>Client: Final response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~90 minutes concerns flagged:
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ndycode
added
the
needs-human-review
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.ts`:
- Around line 2471-2479: The response-id write is racing with the
account-affinity write because onResponseId (capturedResponseId) can arrive
after handleSuccessResponse returns; change to perform an atomic per-request
update in sessionAffinityStore instead of two separate writes: add a new method
like rememberLastSession(sessionAffinityKey, accountId, responseId) (or
rememberLastAccountAndResponse) and call it only once when the final responseId
is available (for non-streaming flow use the immediate responseId, for streaming
wait for onResponseId before calling the atomic method); update usages around
handleSuccessResponse, capturedResponseId,
sessionAffinityStore.rememberLastResponseId and the synchronous account-affinity
write to use the new atomic call, and add a vitest that issues two concurrent
requests using the same sessionAffinityKey to assert the store ends up with
matching accountId+responseId (cover both code paths referenced near the other
occurrence around the second block).
- Around line 1350-1359: The response-continuation feature is erroneously gated
on sessionAffinityStore in the shouldUseResponseContinuation logic (see
parsedBody, shouldUseResponseContinuation,
getResponseContinuation(pluginConfig), sessionAffinityStore, getLastResponseId,
continuationSessionKey, parsedBody.previous_response_id); update the logic to
not silently no-op when response continuation is enabled without session
affinity by either (A) ensuring the session affinity store is instantiated/kept
alive whenever getResponseContinuation(pluginConfig) is true so
getLastResponseId/capture callbacks work, or (B) throw a clear configuration
error/fail-fast when responseContinuation=true while session affinity is
disabled; also add a vitest that enables responseContinuation=true with session
affinity disabled to assert the new behavior.
In `@test/index.test.ts`:
- Around line 1324-1333: The tests currently use buildRoutingManager() via
AccountManager.loadFromDisk.mockResolvedValueOnce(...) which advances selection
on each lookup and makes the two-call happy-path non-idempotent; change the stub
so the primary-account lookup is deterministic across independent sdk.fetch()
calls by returning an idempotent routing manager (or mock that returns the same
account every time) instead of a selection-advancing instance—update the mocks
at AccountManager.loadFromDisk (and the second occurrence) to mockResolvedValue
(or explicitly mock the primary-selection method) so consecutive requests see
the same account, and keep buildRoutingManager only for the failover cases that
need traversal behavior.
- Around line 1319-1432: The tests only inspect the argument passed into
fetchHelpers.transformRequestForCodex and miss whether previous_response_id
survives until the actual outbound request; update both tests to parse and
assert the JSON body passed to the mocked globalThis.fetch (inspect
vi.mocked(globalThis.fetch).mock.calls[...] [0/1][0/1].body) to ensure
previous_response_id is present/absent as expected, and add a new regression
test that fires two sdk.fetch calls concurrently with the same prompt_cache_key
to verify the continuation store doesn’t race (use Promise.all to run both
requests and then assert the two fetch calls’ bodies reflect correct
preservation/override of previous_response_id). Reference
transformRequestForCodex, handleSuccessResponse, sdk.fetch, prompt_cache_key and
previous_response_id when locating where to add/modify assertions.
In `@test/plugin-config.test.ts`:
- Line 193: Add a deterministic vitest regression that verifies loading
disk-backed plugin config preserves responseContinuation when true: call the
existing loadPluginConfig helper (or the module-level loadPluginConfig function)
with a temporary file containing {"responseContinuation": true} and assert the
returned config.responseContinuation === true; place this test alongside the
other plugin-config.test cases so it fails if the file-path parsing drops the
flag, and ensure it uses vitest's temporary file/cleanup utilities to remain
deterministic on Windows and in concurrent runs.
In `@test/session-affinity.test.ts`:
- Around line 126-135: Add a regression test to verify that
SessionAffinityStore.remember(...) does not clear an existing lastResponseId:
create a store, call remember("session-a", 1, t1), call
rememberLastResponseId("session-a", "resp_123", t2), then call
remember("session-a", 2, t3) to update the account index and assert that
getLastResponseId("session-a", t4) still returns "resp_123" and
getPreferredAccountIndex("session-a", t4) returns 2; place this alongside the
existing session-affinity tests to cover the behavior implemented in remember()
/ rememberLastResponseId().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 87860fb3-bfde-451d-844b-4f6a5aa63dbe
📒 Files selected for processing (12)
index.tslib/config.tslib/request/fetch-helpers.tslib/request/response-handler.tslib/schemas.tslib/session-affinity.tslib/types.tstest/index.test.tstest/plugin-config.test.tstest/request-transformer.test.tstest/response-handler.test.tstest/session-affinity.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/schemas.tslib/config.tslib/types.tslib/request/fetch-helpers.tslib/session-affinity.tslib/request/response-handler.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/request-transformer.test.tstest/session-affinity.test.tstest/plugin-config.test.tstest/response-handler.test.tstest/index.test.ts
🔇 Additional comments (17)
lib/config.ts (2)
151-151: lgtm - default config addition follows existing pattern.the new
responseContinuation: falsedefault is positioned correctly within the config object.
921-937: lgtm - config accessor follows established conventions.
getResponseContinuationcorrectly usesresolveBooleanSettingwith env overrideCODEX_AUTH_RESPONSE_CONTINUATION, matching the pattern of other boolean config getters in this file.lib/types.ts (2)
35-55: lgtm - type definitions cover known format variants with extensibility.the
TextFormatConfigunion handlestext,json_object,json_schema, plus a generic fallback. index signatures allow forward compatibility with new api fields.
124-124: lgtm - request body extended with responses api contract fields.
prompt_cache_retentionandprevious_response_idalign with the response continuation feature.text.formattyping matches upstream api.Also applies to: 133-136
test/request-transformer.test.ts (1)
615-659: lgtm - regression tests cover field preservation through request transformation.these tests verify
previous_response_id,prompt_cache_retention, andtext.formatsurvive the transform pipeline. deterministic and use vitest correctly.lib/session-affinity.ts (3)
69-81: lgtm - preserveslastResponseIdwhen updating account index.correctly retrieves existing entry before overwrite to carry forward the stored response id. this prevents losing continuation state during account rotation.
84-98: lgtm -getLastResponseIdcorrectly handles expiry.deletes expired entries lazily and returns trimmed response id or null. mirrors the pattern used by
getPreferredAccountIndex.
100-122: ttl refresh on everyrememberLastResponseIdcall is intentional; verify test coverage.line 118 resets
expiresAt = now + this.ttlMson each response id capture. this is a deliberate sliding-window design: active sessions stay live as long as they're actively used. the pattern matchesremember()at line 78—reads don't refresh, only writes do. integration tests attest/index.test.ts:1334-1392confirmresponseContinuationfeature works correctly across multiple turns.the
prune()method (lib/session-affinity.ts:160-169) provides explicit cleanup for truly inactive entries. unit test attest/session-affinity.test.ts:117-124covers basic response id storage; consider adding an explicit test case for "ttl refresh prevents expiry on update" to document the sliding-window behavior.test/session-affinity.test.ts (1)
117-124: lgtm - covers happy path for response id storage/retrieval.deterministic test with explicit timestamps. verifies both
getLastResponseIdand thatgetPreferredAccountIndexstill works after response id update.lib/request/response-handler.ts (4)
11-33: lgtm - safe callback invocation with error handling.
notifyResponseIdwraps the callback in try/catch to prevent upstream errors from crashing the response flow. logging theresponseIdon failure is acceptable - these are opaque upstream identifiers, not auth tokens.
35-50: lgtm - unified terminal event handling.
maybeCaptureResponseEventcorrectly handles bothresponse.doneandresponse.completedevent types, extracting and notifying the response id fromdata.response.
162-204: lgtm - streaming capture implementation is correct.
createResponseIdCapturingStream:
- buffers partial lines until newline (line 172 keeps tail)
- forwards chunks unchanged via
controller.enqueue(chunk)(line 196)- handles final incomplete line in
flush()(lines 199-200)- silently ignores malformed json to preserve stream integrity
274-292: streaming path is correct - body not consumed before capture.verified lib/request/fetch-helpers.ts:868:
attachResponseIdCapturereceives unconsumed response body directly fromhandleSuccessResponse. header operations (response.headers.get, ensureContentType) don't consume the body. test coverage at test/response-handler.test.ts:186-204 confirms sse stream preservation works as expected. no changes needed.test/response-handler.test.ts (2)
119-130: lgtm - testsonResponseIdcallback in json conversion path.uses
vi.fn()to verify callback invocation with correct response id. also confirms the json body still contains the expected payload.
186-205: lgtm - tests streaming capture preserves content and invokes callback.verifies:
- original sse text is unchanged after transformation
onResponseIdcallback receives correct idcontent-typeheader is preservedlib/schemas.ts (1)
50-50: lgtm - schema matches config and defaults.
responseContinuation: z.boolean().optional()aligns withlib/config.ts:151default andlib/config.ts:931-937accessor.lib/request/fetch-helpers.ts (1)
848-868: nice success-path plumbing here.both branches now forward
onResponseIdthrough the existing capture points inlib/request/response-handler.ts:20-33andlib/request/response-handler.ts:162-204, so json-converted and pass-through streams stay consistent.
|
@coderabbitai review |
|
✅ Actions performedReview triggered.
|
ndycode
removed
the
needs-human-review
ndycode
dismissed
coderabbitai[bot]’s stale review
All review threads are resolved and later commits addressed this stale automated change request.
2 participants
Summary
previous_response_id,text.format, andprompt_cache_retentionin the Responses request contractStack
#309